Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added Interface for AdminUrlGenerator and replaced declarations in pr… #5909

Conversation

bytes-commerce
Copy link
Contributor

…operties and constructors.

Follow up to #5901

This PR should introduce a yet not existing Interface that can be used to Mock the class in external Unit Tests.

This should act as a kind of proof of concept in order to make the EasyAdmin project more strict, yet extensible and testable.

@bytes-commerce
Copy link
Contributor Author

bytes-commerce commented Sep 4, 2023

Seems like there is some flakyness? Also, it would be good if the project ships a Development Dockerfile that can be used to run quality tools directly in the IDE rather than having to rely on the (eventually expensive) GitHub actions - would ease the development a bit.

@javiereguiluz looks like I am having a hard time to get the tests passing as they throw arbitrary errors. Can you check if I made a mistake or if its an issue with GH and replay the failed ones? 🤔 thanks.

.gitignore Outdated Show resolved Hide resolved
Copy link
Collaborator

@OskarStark OskarStark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me overall.

Do we need to change something regarding the Autowiring?

src/Router/AdminUrlGenerator.php Show resolved Hide resolved
src/Router/AdminUrlGenerator.php Show resolved Hide resolved
src/Router/AdminUrlGeneratorInterface.php Show resolved Hide resolved
@OskarStark
Copy link
Collaborator

@javiereguiluz after merge, I would propose a new release to see how things are going 👍🏻

After some time, I could check it myself easy, by using the interface, create more interfaces

@bytes-commerce
Copy link
Contributor Author

@OskarStark Autowire and Autoconfigure are taking care of it automatically, as we only use this Interface for this very class, Symfony detects it automatically.

@OskarStark
Copy link
Collaborator

While this is true for services in src/ it is not for vendor. @javiereguiluz do you register the class somewhere? If yes we need to alias the Interface there too

@bytes-commerce
Copy link
Contributor Author

Can I have an update on whats the planned procedure here now? If this PR is going to get merged I'd prepare more changes to further improve the application. 👍

@javiereguiluz
Copy link
Collaborator

Hi! Sorry for the lack of response here 🙏 I need more time to review this. Thanks!

@bytes-commerce
Copy link
Contributor Author

@javiereguiluz please ask me anything you want to know about it in here - I want to propose many more changes with basic interfaces before proposing bigger changes (to simplify, or work with composite patterns etc), and it just makes sense that everyone involved is on the same level on the matter. Also - great chance to learn a thing or two for everyone, I guess. 👍 So, take your time, but please do not forget about me :-)

@bytes-commerce
Copy link
Contributor Author

Another 2 weeks, whats the status of this? @javiereguiluz

@javiereguiluz
Copy link
Collaborator

I promise I haven't forgotten about you or your PR 🙏 These past days I've been very busy working on some improvement for EasyAdmin. It's mostly internal, and not trivial, so it's taking me a lot of time. I'll try to find some time for this too.

@bytes-commerce
Copy link
Contributor Author

@javiereguiluz I will ping you in another few weeks then! 🙏 happy coding in the meantime!!

@javiereguiluz javiereguiluz added this to the 4.x milestone Oct 22, 2023
@javiereguiluz javiereguiluz force-pushed the interface-adaption-admin-url-generator branch from 97d022f to b07cea4 Compare October 22, 2023 08:54
@javiereguiluz javiereguiluz merged commit b704f5e into EasyCorp:4.x Oct 22, 2023
17 of 19 checks passed
@javiereguiluz
Copy link
Collaborator

Max, I finally could merge your PR. Thanks again for submitting it and for your patience during this time. Cheers!

@bytes-commerce
Copy link
Contributor Author

@javiereguiluz 🥳 amazing! I will observe the situation for now and eventually present new Interfaces for final classes if thinks go as expected 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants